Repository navigation
NodesList: look nodes up under the same monitor as add and remove - #22
Merged
Merged
Conversation
contains and nodeById read the trove id index without a lock, while add, remove and compact change it under the list's monitor. A lookup that races an add which rehashes the index can miss a node that is in the list. Pass writers add nodes while the pass's parts traverse the graph, so a NodeRef built from an id, such as the OpNodes Assignment wrapper, could come out empty. Its first property read then tried to load the node from disk, where it had never been written, and failed with "unable to read node from disk". atom crashed this way in 9 of 10 usages runs on node-express-realworld-example-app with TypeScript types on. The lookups now take the monitor, and so does access to the label map, which readers also changed through computeIfAbsent. The lists handed out by nodesByLabel are still live and not safe to iterate during concurrent adds; that is left for a separate change. With this, the same project passes 30 of 30 runs with byte-identical usages, and juice-shop usages and reachables take the same time as before. The new NodesListTest case fails without the change. Bump to 3.1.1. Signed-off-by: Team AppThreat <cloud@appthreat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
atom crashed intermittently with
unable to read node from diskwhile jssrc2cpg's import resolver ran: 9 of 10 usages runs on node-express-realworld-example-app with TypeScript types on.The cause is in NodesList.
containsandnodeByIdread the trove id index without a lock, whileadd,removeandcompactchange it under the list's monitor. A lookup that races an add which rehashes the index can miss a node that is in the list. Pass writers add nodes while the pass's parts traverse the graph, so a NodeRef built from an id, such as chen'sOpNodes.Assignmentwrapper, could come out empty. Its first property read then went to disk for a node that had never been written there.The lookups now take the monitor, and so does access to the label map, which readers also changed through
computeIfAbsent. The lists returned bynodesByLabelare still live, so iterating them while another thread adds nodes of that label remains unsafe. I haven't seen that fail, and it is left for a separate change.With the fix, the same project passes 30 of 30 runs with byte-identical output. The other four corpus repos pass 10 of 10 each. juice-shop usages and reachables take the same time as before (11 to 17 s either way) and give identical slices. The new
NodesListTestcase fails every time without the change. Version bumped to 3.1.1; cpg2 and chen need to follow for atom to pick it up.